-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOCS-1637: Make ultrasonic
sensor page in micro-RDK
#2614
DOCS-1637: Make ultrasonic
sensor page in micro-RDK
#2614
Conversation
Overall readability score: 55.54 (🔴 -0.05)
View detailed metrics🟢 - Shows an increase in readability
Averages:
View metric targets
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % tiny comments. Thank you!
@@ -8,13 +8,14 @@ The micro-RDK is a lightweight version of the {{% glossary_tooltip term_id="rdk" | |||
|
|||
The only microcontroller the micro-RDK currently supports is the [ESP32](https://www.espressif.com/en/products/socs/esp32). | |||
|
|||
[Client API](/build/program/apis/) usage with the micro-RDK currently supports only the following {{< glossary_tooltip term_id="resource" text="resources" >}}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great change!
Co-authored-by: andf-viam <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, only a few minor comments.
linkTitle: "Sensor" | ||
weight: 30 | ||
type: "docs" | ||
description: "Support in the micro-RDK for sensors, devices that measur information about the outside world." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sp. measure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! thanks
{ | ||
"trigger_pin": "<pin-number>", | ||
"echo_interrupt_pin": "<pin-number>", | ||
"timeout_ms": <int> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a convention for indicating optional parameters in an attributes template like this? If so, timeout_ms
should be noted as optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, but re. later comment removed this
{ | ||
"trigger_pin": "15", | ||
"echo_interrupt_pin": "18", | ||
"timeout_ms": "200" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For most purposes with this sensor it should be fine to select the default timeout and not specify one. Maybe this should be omitted? Similar for other examples with an explicit value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it from the examples
| Attribute | Type | Inclusion | Description | | ||
| --------- | ---- | --------- | ----------- | | ||
| `trigger_pin` | string | **Required** | The GPIO number of the [board's](/build/micro-rdk/board/) GPIO pin that you have wired to the trigger pin of your ultrasonic sensor. | | ||
| `echo_interrupt_pin` | string | **Required** | The GPIO number of the board's GPIO pin that you have wired to the echo pin of your ultrasonic sensor. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the RDK usonic sensor, the micro-rdk one cannot use a digital interrupt, and I think it is worth noting that important distinction here. The language in the comments in the implementation say this: Please note that unlike the RDK ultrasonic sensor, you must not use a named pin associated with a digital interrupt configured on the board: it will not (currently) work.
.
| --------- | ---- | --------- | ----------- | | ||
| `trigger_pin` | string | **Required** | The GPIO number of the [board's](/build/micro-rdk/board/) GPIO pin that you have wired to the trigger pin of your ultrasonic sensor. | | ||
| `echo_interrupt_pin` | string | **Required** | The GPIO number of the board's GPIO pin that you have wired to the echo pin of your ultrasonic sensor. | | ||
| `timeout_ms` | int | Optional | Time to wait in milliseconds before initiating a timeout when requesting readings from your ultrasonic sensor. <br> Default: `1000`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default timeout for the micro-rdk usonic sensor is actually 50ms, and it is clamped to 100ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by clamped, it can't go any higher than?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. Even if you put in 1000 it will internally limit the timeout to 100ms. The implementation in the micro-rdk is currently blocking. The sensor device itself never needs more than, say, 60ms. But if you set the timeout to 10000ms or something you would basically hang the micro-rdk, so we ignore any timeout above 100ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after integrating the proposed edit.
| --------- | ---- | --------- | ----------- | | ||
| `trigger_pin` | string | **Required** | The GPIO number of the [board's](/build/micro-rdk/board/) GPIO pin that you have wired to the trigger pin of your ultrasonic sensor. | | ||
| `echo_interrupt_pin` | string | **Required** | The GPIO number of the board's GPIO pin that you have wired to the echo pin of your ultrasonic sensor. | | ||
| `timeout_ms` | int | Optional | Time to wait in milliseconds before initiating a timeout when requesting readings from your ultrasonic sensor. <br> Default: `1000`. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. Even if you put in 1000 it will internally limit the timeout to 100ms. The implementation in the micro-rdk is currently blocking. The sensor device itself never needs more than, say, 60ms. But if you set the timeout to 10000ms or something you would basically hang the micro-rdk, so we ignore any timeout above 100ms.
You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2614 |
Key difference from RDK is that it doesn't have board attribute-- @acmorrow noticed config builder threw error for board attribute missing from JSON, is that ok?